Code Review: PR #576 — feat(yeoman-ui): implement generator progress notifications

Overview

The PR replaces the single "Installing dependencies..." notification with a phased progress notification driven by yeoman lifecycle events (method:writing, method:install, method:end). It adds a project name to the title (Generating {projectName}) and success message, keeps one long-lived withProgress notification alive via a stored progressReporter, and updates doGeneratorDone to return a Thenable so the caller can await the done message. It also hardens tests against real filesystem writes.

The user-facing goal is reasonable and the test additions are welcome. However, there are a few correctness concerns worth resolving before merge.

🔴 Significant Issues

1. Phase messages can render out of order (race between fixed delays)

vscode-youi-events.ts:144-155

Each yeoman event handler calls void doGeneratorProgress(...) without awaiting (yeomanui.ts:598-613), so three independent async calls run concurrently, each with its own setTimeout:

• install reports after await 2000ms + 10ms
• end reports after 10ms

For a fast/no-op install, method:end fires shortly after method:install, so the end handler reports "Finalising…" first, and ~2s later the install handler overwrites it with "Installing dependencies…" — the reverse of the intended sequence. The artificial 2s delay is decoupled from actual progress and is the root cause. Consider sequencing the phases (await the chain) or driving the message purely from the latest event rather than fixed timers.

2. Early doClose() on method:writing likely breaks the "closed manually" analytics

vscode-youi-events.ts:138-140 → AbstractWebviewPanel.ts:133-156

The writing phase now calls doClose(), disposing the webview panel. method:writing fires for essentially every generator, whereas the old doGeneratorInstall() only closed the panel for generators that had an install step.

doClose() → panel onDidDispose → AbstractWebviewPanel.dispose(), which reads GENERATOR_COMPLETED. That flag is only set later in doGeneratorDone (vscode-youi-events.ts:107) — and by then this.webviewPanel is already null, so set(null, …) is a no-op. Net effect: on normal completion the panel is disposed during writing with GENERATOR_COMPLETED === undefined, so dispose() treats it as a manual close and fires updateGeneratorClosedManually for successful generations. Please verify this on a generator without an install step — I believe it's a telemetry regression.

3. Non-VSCode (WebSocket) path invokes an RPC with no frontend handler

server-youi-events.ts:48-54

ServerYouiEvents.doGeneratorProgress calls this.rpc.invoke("generatorProgress", …), but App.vue's initRpc function list (App.vue:665-679) has no generatorProgress handler (confirmed by grep — none exists in frontend/). This await will reject/hang for the standalone browser flow. Either add the frontend handler or guard the invocation.

🟡 Moderate Issues

4. doGeneratorInstall appears to be dead code now

onGenInstall no longer calls doGeneratorInstall — it calls doGeneratorProgress for all phases. The only remaining references to doGeneratorInstall are its interface/impl definitions and a test (youi-events.ts:11, vscode-youi-events.ts:119). If it's genuinely unused, remove it (and its test); otherwise document who still calls it.

5. User-facing strings bypass the i18n messages.ts convention

vscode-youi-events.ts:129-133, 392, 399-403

The codebase centralizes strings in messages.ts (artifact_generated_*, etc.). The new strings ("Creating project files…", "Installing dependencies…", "Finalising…", "Generating {name}", "Project {name} has been generated.") are hardcoded inline. This is inconsistent with the existing pattern the PR is otherwise using (this.messages.*) and makes future localization harder. Move them to messages.ts.

6. Fragile timing assumptions

vscode-youi-events.ts:142-143

The 50ms sleep "wait for the progress reporter to be initialized" assumes vscode.window.withProgress's callback runs within 50ms. This is a race; if the reporter isn't set in time, the install report silently no-ops. A promise that resolves when progressReporter is assigned would be deterministic.

🟢 Minor / Style

• Loose typing: progressReporter: any and initialMessage-style comments. VS Code's Progress<{ message?: string; increment?: number }> is the proper type; using it would catch report-shape mistakes (vscode-youi-events.ts:65).
• Duplicated "Finalising…": reported both in doGeneratorDone (line 103) and the end phase (line 132). Given the ordering issue in #1, consider a single source of truth.
• getProjectName heuristics: the 6-way _.get fallback chain (yeomanui.ts:586-595) is pragmatic but undocumented — a brief comment on why these specific paths exist would help maintainers.
• getSuccessInfoMessage duplication: the project-name and fallback branches are near-identical mirrors (vscode-youi-events.ts:395-417). Could collapse by computing the workspace suffix once.

Tests

• Good additions for doGeneratorProgress phases, project-name titles, and success messages, plus the WorkspaceFile stubbing to prevent CI filesystem writes.
• Concern: the install phase test exercises the real await 2000ms, adding ~2s of wall-clock per run. Consider injecting/faking the delay (e.g., sinon fake timers) so the suite stays fast.
• Gap: none of the new tests cover the phase ordering (issue #1) or the early-doClose analytics behavior (issue #2) — the two areas most likely to break. The tests assert each phase in isolation, which is why the ordering bug slips through.

Summary

The feature direction is sound and test coverage is expanded, but I'd hold merge on the three 🔴 items — particularly the out-of-order phase messages (#1) and the early-dispose analytics regression (#2), both of which affect the normal success path for most generators. The i18n and dead-code cleanups (#4, #5) are worth folding in while touching this code.